-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios] duration to time interval fix #8276
[ios] duration to time interval fix #8276
Conversation
@fabian-guerra, thanks for your PR! By analyzing this pull request, we identified @incanus, @boundsj and @1ec5 to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few optional changes, but this PR looks good.
{ | ||
return duration.count(); | ||
std::chrono::nanoseconds nano(duration.count()); | ||
std::chrono::seconds sec = std::chrono::duration_cast<std::chrono::seconds>(nano); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, since this is Objective-C++ code, you could condense these lines somewhat by passing duration.count()
directly into the duration_cast
. You can also use auto
instead of declaring the local variable to be of type std::chrono::seconds
. What you’ve got is totally reasonable, though.
#import <XCTest/XCTest.h> | ||
|
||
#include <mbgl/util/chrono.hpp> | ||
#import "../../darwin/src/NSDate+MGLAdditions.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since NSDate+MGLAdditions.h is shared between iOS and macOS, the tests should be shared as well. Can you move this file to platform/darwin/test/ and add it to macos.xcodeproj?
mbgl::Duration duration = MGLDurationFromTimeInterval(timeInterval); | ||
NSTimeInterval durationTimeInterval = MGLTimeIntervalFromDuration(duration); | ||
|
||
mbgl::Duration durationInNanoseconds(5000000000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since chrono.hpp imports <chrono>
, you can write using namespace std::chrono_literals;
at the top, then use auto expectedDuration = 5000000000ns;
or even mbgl::Duration expectedDuration = 5s;
.
Actually, sharing the tests between iOS and macOS is pretty important, so if you could take care of that, that’d be 👌. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending @1ec5's recommendations.
4d81add
to
fabad6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears to have added three identical MGLNSDateAdditionsTests.mm files, each in a different directory. There should only be one, under platform/darwin/test/.
{ | ||
return duration.count(); | ||
std::chrono::nanoseconds nano(duration.count()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation makes the unsafe assumption that mbgl::Duration
's count is in nanoseconds. mbgl::Duration
is a typedef to std::chrono::steady_clock::duration
, and std::chrono::steady_clock
does not specify a particular tick period.
In addition, this implementation truncates any fractional seconds in the duration. NSTimeInterval
is a floating-point type, and so the conversion should be expected to preserve fractional seconds.
I believe the correct conversion is as follows (but don't trust me -- write some more tests 😄):
NSTimeInterval MGLTimeIntervalFromDuration(mbgl::Duration duration)
{
std::chrono::duration<NSTimeInterval, std::ratio<1>>(duration).count();
}
Fixes #8264